Merged
Conversation
StarpTech
requested review from
JivusAyrus,
thisisnithin and
wilsonrivera
as code owners
Contributor
WalkthroughReplaces the NewFeaturesPopup root with a Link to the external Hub, updates title/body/CTA and decorative/animation layers, preserves close behavior with preventDefault/stopPropagation, and renames the popup-dismissal localStorage key. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
studio/src/components/dashboard/NewFeaturesPopup.tsx (1)
35-42: Consider the nested interactive elements pattern.The
<Button>is nested inside the<Link>(which renders as an<a>tag). While the event handling correctly prevents functional issues, nesting interactive content inside an anchor is technically invalid HTML and can cause accessibility concerns with screen readers.An alternative approach would be to use a
<div>withrole="button"and appropriate keyboard handling, or position the close button absolutely outside the Link's DOM tree while visually appearing inside it.This is a common pattern and works functionally, so flagging as optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/NewFeaturesPopup.tsx` around lines 35 - 42, The close Button (rendering Cross2Icon) is currently nested inside the Link in NewFeaturesPopup which creates invalid nested interactive elements; update the markup so the close control is not a child of the Link—either move the Button node out of the Link DOM and position it absolutely to visually appear inside the link, or replace the inner Button with a non-semantic element (e.g., a div/span) given role="button" and add keyboard handlers that call handleClosePopup; ensure to keep handleClosePopup as the click/keyboard handler and preserve the existing styles and aria-label for accessibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@studio/src/components/dashboard/NewFeaturesPopup.tsx`:
- Around line 35-42: The close Button (rendering Cross2Icon) is currently nested
inside the Link in NewFeaturesPopup which creates invalid nested interactive
elements; update the markup so the close control is not a child of the
Link—either move the Button node out of the Link DOM and position it absolutely
to visually appear inside the link, or replace the inner Button with a
non-semantic element (e.g., a div/span) given role="button" and add keyboard
handlers that call handleClosePopup; ensure to keep handleClosePopup as the
click/keyboard handler and preserve the existing styles and aria-label for
accessibility.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
studio/src/components/dashboard/NewFeaturesPopup.tsxstudio/src/hooks/use-new-features-popup-disabled.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2570 +/- ##
==========================================
- Coverage 36.94% 1.49% -35.46%
==========================================
Files 964 294 -670
Lines 126281 47248 -79033
Branches 5153 433 -4720
==========================================
- Hits 46657 704 -45953
+ Misses 77981 46260 -31721
+ Partials 1643 284 -1359
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio/src/components/dashboard/NewFeaturesPopup.tsx (1)
23-61:⚠️ Potential issue | 🟠 MajorRemove the nested
Buttonfrom inside theLinkto fix invalid semantic structure.A native
<button>cannot be nested inside an<a>(Link) element—this violates HTML semantics and breaks keyboard navigation and screen-reader behavior. Move the dismiss button outside the anchor and use absolute positioning instead.Suggested structural fix
- <Link + <div href="https://hub.wundergraph.com/?utm_source=cosmo&utm_content=popup&utm_medium=internal" - target="_blank" - rel="noreferrer" className="group relative block w-[195px] overflow-hidden rounded-lg p-[1px] transition-all duration-300 ease-out before:absolute before:inset-0 before:z-[-1] before:rounded-lg before:bg-gradient-to-r before:from-[hsla(271,91%,65%,1)] before:to-[hsla(330,81%,60%,1)] before:opacity-80 before:transition-opacity before:duration-300 before:content-[''] hover:scale-[1.02] hover:shadow-[0_0_24px_-4px_hsla(271,91%,65%,0.4)] hover:before:opacity-100" - > + > + <Link + href="https://hub.wundergraph.com/?utm_source=cosmo&utm_content=popup&utm_medium=internal" + target="_blank" + rel="noreferrer" + className="block h-full w-full" + > <style>{` `@keyframes` popup-shine { 0% { transform: translateX(-100%); } 40% { transform: translateX(200%); } 100% { transform: translateX(200%); } } `}</style> <div className="pointer-events-none absolute inset-0 z-20 overflow-hidden rounded-lg"> ... </div> <div className="relative z-10 flex h-full w-full flex-col items-start justify-center gap-6 rounded-lg bg-card p-3"> <div className="flex w-full flex-col gap-3"> <div className="flex w-full items-start justify-between"> <h2 className="text-base font-semibold leading-tight text-card-foreground"> Discover Hub: Built for schema design & governance </h2> - <Button - variant="ghost" - size="icon" - className="h-4 w-4 cursor-pointer text-muted-foreground opacity-0 transition-all duration-300 group-hover:opacity-100" - onClick={handleClosePopup} - > - <Cross2Icon /> - </Button> </div> ... </div> </div> - </Link> + </Link> + <Button + variant="ghost" + size="icon" + className="absolute right-3 top-3 z-30 h-4 w-4 cursor-pointer text-muted-foreground opacity-0 transition-all duration-300 group-hover:opacity-100 group-focus-within:opacity-100" + onClick={handleClosePopup} + aria-label="Dismiss hub banner" + > + <Cross2Icon /> + </Button> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/NewFeaturesPopup.tsx` around lines 23 - 61, The dismiss Button nested inside the Link component creates invalid HTML (a button inside an anchor); move the Button component (the one rendering Cross2Icon and calling handleClosePopup) out of the Link block and position it absolutely over the card instead—locate the Link usage and the nested Button in NewFeaturesPopup (the Link wrapping the gradient and content) and extract the Button so it is a sibling of the Link (or sibling of the surrounding container) while preserving its classes/props (variant="ghost", size="icon", className including absolute positioning) and its onClick handler (handleClosePopup) to maintain the same visual placement and behavior for keyboard/screen-reader accessibility.
🧹 Nitpick comments (1)
studio/src/components/dashboard/NewFeaturesPopup.tsx (1)
29-45: Movepopup-shinestyles out of inline JSX.The inline
<style>+ inline animation declaration is harder to maintain and can conflict with stricter CSP setups. Prefer a CSS/Tailwind-defined keyframe class and apply it viaclassName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/NewFeaturesPopup.tsx` around lines 29 - 45, The inline <style> block and inline animation on the shimmer div (popup-shine) should be moved into a stylesheet/Tailwind utility: define an `@keyframes` popup-shine and a corresponding CSS class (e.g., .animate-popup-shine or a Tailwind plugin/extend animation entry) in your component stylesheet or global CSS/Tailwind config, move the background gradient into that class as well (or into a named utility class) and then remove the inline <style> and the inline animation property, applying the new class via className on the same element in NewFeaturesPopup so CSP-compliant styling is used instead of inline styles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@studio/src/components/dashboard/NewFeaturesPopup.tsx`:
- Around line 23-61: The dismiss Button nested inside the Link component creates
invalid HTML (a button inside an anchor); move the Button component (the one
rendering Cross2Icon and calling handleClosePopup) out of the Link block and
position it absolutely over the card instead—locate the Link usage and the
nested Button in NewFeaturesPopup (the Link wrapping the gradient and content)
and extract the Button so it is a sibling of the Link (or sibling of the
surrounding container) while preserving its classes/props (variant="ghost",
size="icon", className including absolute positioning) and its onClick handler
(handleClosePopup) to maintain the same visual placement and behavior for
keyboard/screen-reader accessibility.
---
Nitpick comments:
In `@studio/src/components/dashboard/NewFeaturesPopup.tsx`:
- Around line 29-45: The inline <style> block and inline animation on the
shimmer div (popup-shine) should be moved into a stylesheet/Tailwind utility:
define an `@keyframes` popup-shine and a corresponding CSS class (e.g.,
.animate-popup-shine or a Tailwind plugin/extend animation entry) in your
component stylesheet or global CSS/Tailwind config, move the background gradient
into that class as well (or into a named utility class) and then remove the
inline <style> and the inline animation property, applying the new class via
className on the same element in NewFeaturesPopup so CSP-compliant styling is
used instead of inline styles.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
studio/src/components/dashboard/NewFeaturesPopup.tsx (1)
23-57:⚠️ Potential issue | 🟠 MajorDon’t nest the close
<Button>inside the root<Link>.Line 23 makes the entire card an anchor, and Line 52 places another interactive control inside it. This is invalid interactive nesting and can break keyboard/screen-reader behavior for dismissal.
💡 Suggested structure (link + close button as siblings)
- return ( - <Link + return ( + <div + className="group relative block w-[195px] overflow-hidden rounded-lg p-[1px] transition-all duration-300 ease-out before:absolute before:inset-0 before:z-[-1] before:rounded-lg before:bg-gradient-to-r before:from-[hsla(271,91%,65%,1)] before:to-[hsla(330,81%,60%,1)] before:opacity-80 before:transition-opacity before:duration-300 before:content-[''] hover:scale-[1.02] hover:shadow-[0_0_24px_-4px_hsla(271,91%,65%,0.4)] hover:before:opacity-100" + > + <Link href="https://hub.wundergraph.com/login?utm_source=cosmo&utm_content=popup&utm_medium=internal" target="_blank" rel="noreferrer" - className="group relative block w-[195px] overflow-hidden rounded-lg p-[1px] transition-all duration-300 ease-out before:absolute before:inset-0 before:z-[-1] before:rounded-lg before:bg-gradient-to-r before:from-[hsla(271,91%,65%,1)] before:to-[hsla(330,81%,60%,1)] before:opacity-80 before:transition-opacity before:duration-300 before:content-[''] hover:scale-[1.02] hover:shadow-[0_0_24px_-4px_hsla(271,91%,65%,0.4)] hover:before:opacity-100" - > + className="block" + > ... - <Button - variant="ghost" - size="icon" - className="h-4 w-4 cursor-pointer text-muted-foreground opacity-0 transition-all duration-300 group-hover:opacity-100" - onClick={handleClosePopup} - > - <Cross2Icon /> - </Button> ... - </Link> + </Link> + + <Button + variant="ghost" + size="icon" + className="absolute right-3 top-3 z-50 h-4 w-4 cursor-pointer text-muted-foreground opacity-0 transition-all duration-300 group-hover:opacity-100" + onClick={handleClosePopup} + > + <Cross2Icon /> + </Button> + </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/dashboard/NewFeaturesPopup.tsx` around lines 23 - 57, The close Button is currently nested inside the root Link (interactive nesting); fix NewFeaturesPopup.tsx by moving the Button (the one that calls handleClosePopup) out of the Link so the Link and the Button are siblings; keep the Link wrapping the card content only (everything except the Button), preserve the Button's variant/size/classes and the onClick handler (handleClosePopup), and ensure the Button has an accessible label (aria-label or visible text) so keyboard/screen-reader dismissal still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@studio/src/components/dashboard/NewFeaturesPopup.tsx`:
- Around line 23-57: The close Button is currently nested inside the root Link
(interactive nesting); fix NewFeaturesPopup.tsx by moving the Button (the one
that calls handleClosePopup) out of the Link so the Link and the Button are
siblings; keep the Link wrapping the card content only (everything except the
Button), preserve the Button's variant/size/classes and the onClick handler
(handleClosePopup), and ensure the Button has an accessible label (aria-label or
visible text) so keyboard/screen-reader dismissal still works.
JivusAyrus
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI/UX Updates
Checklist